Enable NRT for `Serialization` namespace #772

axunonb · 2025-04-15T13:49:55Z

Now all library files have NRT enabled
DataTypeSerializerFactory simplified
UtcOffsetSerializer.GetOffset(string) simplified

codecov · 2025-04-15T13:51:47Z

Codecov Report

Attention: Patch coverage is 40.65041% with 146 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
Ical.Net/Serialization/SerializerBase.cs	20%	12 Missing and 4 partials ⚠️
Ical.Net/Serialization/DataTypes/EnumSerializer.cs	0%	9 Missing ⚠️
...Serialization/DataTypes/RequestStatusSerializer.cs	18%	4 Missing and 5 partials ⚠️
...al.Net/Serialization/DataTypes/StringSerializer.cs	53%	5 Missing and 4 partials ⚠️
...alization/DataTypes/EncodableDataTypeSerializer.cs	20%	4 Missing and 4 partials ⚠️
...l.Net/Serialization/DataTypes/TriggerSerializer.cs	11%	5 Missing and 3 partials ⚠️
Ical.Net/Serialization/DataTypeMapper.cs	12%	1 Missing and 6 partials ⚠️
...l.Net/Serialization/DataTypes/WeekDaySerializer.cs	30%	3 Missing and 4 partials ⚠️
...l.Net/Serialization/DataTypes/IntegerSerializer.cs	25%	3 Missing and 3 partials ⚠️
...Net/Serialization/DataTypes/OrganizerSerializer.cs	45%	3 Missing and 3 partials ⚠️
... and 22 more

❌ Your patch status has failed because the patch coverage (41%) is below the target coverage (80%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (66%) is below the target coverage (80%). You can increase the head coverage or adjust the target coverage.

@@         Coverage Diff         @@
##           main   #772   +/-   ##
===================================
- Coverage    67%    66%   -1%     
===================================
  Files       104    104           
  Lines      4483   4495   +12     
  Branches   1103   1140   +37     
===================================
- Hits       2994   2969   -25     
+ Misses     1064   1061    -3     
- Partials    425    465   +40

Files with missing lines	Coverage Δ
Ical.Net/DataTypes/RequestStatus.cs	`21% <ø> (ø)`
Ical.Net/Serialization/CalendarComponentFactory.cs	`80% <ø> (ø)`
....Net/Serialization/DataTypes/DataTypeSerializer.cs	`71% <100%> (ø)`
....Net/Serialization/DataTypes/DurationSerializer.cs	`86% <ø> (ø)`
Ical.Net/Serialization/SerializationUtil.cs	`87% <100%> (ø)`
Ical.Net/Serialization/SerializerFactory.cs	`81% <100%> (ø)`
Ical.Net/Serialization/SimpleDeserializer.cs	`95% <100%> (ø)`
Ical.Net/DataTypes/StatusCode.cs	`19% <0%> (ø)`
Ical.Net/Serialization/CalendarSerializer.cs	`32% <50%> (ø)`
....Net/Serialization/DataTypes/DateTimeSerializer.cs	`83% <0%> (-2%)`	⬇️
... and 29 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

minichma

Wow, that's a lot of changes. LGTM, just a few thoughts. Skimmed over the code only very briefly though.

minichma · 2025-04-23T15:55:07Z

Ical.Net/Serialization/DataTypes/AttachmentSerializer.cs

            var uriValue = Decode(a, attachment);
-            a.Uri = new Uri(uriValue, UriKind.RelativeOrAbsolute);

+            if (string.IsNullOrEmpty(uriValue)) return null;


Not sure, returning null is the best option here, is it? Anyhow, it's a functional change, right?

I will have another look.

Not sure, returning null is the best option

I believe returning null to avoid throwing exceptions is overused in the codebase. Methods like TryGet(...) would have been a better alternative to returning a NRT. That said, enabling nullable has introduced more null checks but reduced overall coverage. However, this investment will pay off in terms of future code quality.
I've migrated projects to NRT several times, but this particular migration significantly exceeded my initial workload estimates. And that's without the unit tests still ahead of us.

returned value can't be null here, will update the code.
Even if the uriValue were null, it would throw, then the exception is eaten up and null ist returned anyway

minichma · 2025-04-23T15:55:45Z

Ical.Net/Serialization/DataTypes/AttendeeSerializer.cs

        try
        {
-            var a = CreateAndAssociate() as Attendee;
+            if (CreateAndAssociate() is not Attendee a) return null;


rather throw?

We have different implementations using this pattern. Leave the current behavior for now.

minichma · 2025-04-23T15:57:01Z

Ical.Net/Serialization/DataTypes/AttendeeSerializer.cs

+
            var uriString = Unescape(Decode(a, attendee));

+            if (uriString == null) return a;


Here the behavior is different to AttachmentSerializer, where we return null. I think, this way is better.

Returned value can't be null here when args are not null.

minichma · 2025-04-23T15:57:45Z

Ical.Net/Serialization/DataTypes/AttendeeSerializer.cs


-    public override object Deserialize(TextReader tr) => Deserialize(tr.ReadToEnd());
-}
+    public override object? Deserialize(TextReader? tr) => tr != null ? Deserialize(tr.ReadToEnd()) : null;


Why would it be allowed to pass null as the TextReader? Rather make it not nullable?

It's because of overriding the base method, SerializeToString(object? obj) which is nullable

minichma · 2025-04-23T16:06:09Z

Ical.Net/Serialization/DataTypes/UtcOffsetSerializer.cs

-            rawOffset = rawOffset.Substring(1, rawOffset.Length - 1);
-        }
+        // Supported formats
+        var formats = new[] { "hhmmss", "hhmm", "hh" };


Could be declared as static property or field

minichma · 2025-04-23T16:07:36Z

Ical.Net/Serialization/DataTypes/WeekDaySerializer.cs

+        try
+        {
+            var name = Enum.GetName(typeof(DayOfWeek), ds.DayOfWeek);
+            value += name!.ToUpper().Substring(0, 2);


If name is null, it would throw and be catched silently, which is ok, but not too readable. Handle explicitly?

Agree, if (name == null) return null; is more readable

sonarqubecloud · 2025-04-28T08:25:33Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

axunonb force-pushed the wip/axunonb/pr/nrt-serialization branch 2 times, most recently from e7a31be to 8112d39 Compare April 15, 2025 14:19

Enable NRT for Serialization namespace

1feaa17

axunonb force-pushed the wip/axunonb/pr/nrt-serialization branch from 8112d39 to 1feaa17 Compare April 18, 2025 08:04

axunonb marked this pull request as ready for review April 22, 2025 20:19

axunonb requested a review from minichma April 22, 2025 20:20

minichma previously approved these changes Apr 23, 2025

View reviewed changes

axunonb added 2 commits April 27, 2025 13:28

Merge branch 'main' into wip/axunonb/pr/nrt-serialization

bee7d3c

Implemented changes from review

c5dbc5b

axunonb dismissed minichma’s stale review via c5dbc5b April 27, 2025 12:20

axunonb requested a review from minichma April 27, 2025 12:21

minichma approved these changes Apr 27, 2025

View reviewed changes

Merge branch 'main' into wip/axunonb/pr/nrt-serialization

8e5e57e

axunonb merged commit fb2ce04 into main Apr 28, 2025
7 of 9 checks passed

axunonb deleted the wip/axunonb/pr/nrt-serialization branch April 28, 2025 08:26

This was referenced Jul 28, 2025

Bump the all-updates group with 4 updates mikkelkaas/HaandboldGrabber#13

Closed

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#243

Closed

dependabot bot mentioned this pull request Aug 4, 2025

Bump Ical.Net and 5 others Atrejoe/Inky-Calendar-Server#245

Closed

dependabot bot mentioned this pull request Aug 18, 2025

Bump the all-updates group with 4 updates mikkelkaas/HaandboldGrabber#14

Merged

dependabot bot mentioned this pull request Aug 25, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#246

Closed

dependabot bot mentioned this pull request Sep 1, 2025

Bump Ical.Net from 4.3.1 to 5.1.0 Atrejoe/Inky-Calendar-Server#253

Closed

dependabot bot mentioned this pull request Oct 13, 2025

Bump Ical.Net from 4.3.1 to 5.1.1 Atrejoe/Inky-Calendar-Server#257

Open

This was referenced Oct 23, 2025

Bump Ical.Net from 4.3.1 to 5.1.1 joinrpg/joinrpg-net#3675

Closed

Bump Ical.Net from 4.3.1 to 5.1.1 loresoft/InstructorIQ#379

Merged

Bump Ical.Net from 4.3.1 to 5.1.1 joinrpg/joinrpg-net#3707

Closed


		var uriString = Unescape(Decode(a, attendee));

		if (uriString == null) return a;

Enable NRT for Serialization namespace #772

Enable NRT for Serialization namespace #772

Uh oh!

Conversation

axunonb commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

minichma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axunonb Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Apr 28, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Enable NRT for `Serialization` namespace #772

Enable NRT for `Serialization` namespace #772

axunonb commented Apr 15, 2025 •

edited

Loading

codecov bot commented Apr 15, 2025 •

edited

Loading

axunonb Apr 27, 2025 •

edited

Loading

axunonb Apr 27, 2025 •

edited

Loading